Skip to content

resolver: merge experimentalDecorators across tsconfig extends chain - #30478

Closed
robobun wants to merge 8 commits into
mainfrom
farm/31c67eff/merge-experimental-decorators-across-extends
Closed

robobun wants to merge 8 commits into
mainfrom
farm/31c67eff/merge-experimental-decorators-across-extends

Conversation

@robobun

@robobun robobun commented May 11, 2026

Copy link
Copy Markdown
Collaborator

Fixes #30477.

Problem

A tsconfig that sets experimentalDecorators: true and extends a parent that does not set it silently loses the flag:

// base-tsconfig.json
{ "compilerOptions": { "target": "esnext" } }

// tsconfig.json
{
  "extends": "./base-tsconfig.json",
  "compilerOptions": { "experimentalDecorators": true }
}

Bun emits stage-3 (TC39) decorators instead of legacy ones, so reflect-metadata throws TypeError and real-world decorator codebases break on 1.3.10+. Worked in 1.3.5–1.3.9.

Root cause

PR #26436 introduced the TC39 decorators pipeline and added TSConfigJSON.experimental_decorators — parsed at tsconfig_json.zig:182, consumed at resolver.zig:1067 — but did not add the corresponding merge in the extends-chain loop at resolver.zig:4253. The neighbouring emit_decorator_metadata is OR-merged on the line just above; experimental_decorators was missed. Since the loop's merged_config starts as the base config (experimentalDecorators: false by default) and the child's value never gets OR'd in, the child's true is discarded.

Fix

One-line merge matching the pattern already used for emit_decorator_metadata:

merged_config.experimental_decorators =
    merged_config.experimental_decorators or parent_config.experimental_decorators;

Drive-by fixes for --tsconfig-override

The reproducer in #30477 also uses --tsconfig-override ./tsconfig.bun.json, which surfaced two adjacent bugs:

  1. Internal error: directory mismatch warning — parseTSConfig was called with the directory-fd of the dir being iterated, but the override path may live outside that dir. openat(dirname_fd, basename(path)) in cache.zig fails with ENOENT and falls back to an absolute-path open, logging the warning along the way. Fix: pass .invalid when loading an override at the root-directory slot.

  2. Override ignored for decorator defaults — the override attaches to the filesystem root (parent == null in dirInfoUncached). Children inherit it via enclosing_tsconfig_json, not tsconfig_json. transpiler.configureLinker only checked root_dir.tsconfig_json, so it read null for the working directory and left experimental_decorators / emit_decorator_metadata unset. Fix: fall back to enclosing_tsconfig_json.

This second fix also benefits the plain case where a tsconfig lives in an ancestor directory (also previously silently ignored by the transpiler's defaults).

Verification

test/regression/issue/30477.test.ts covers:

  • experimentalDecorators: true in the child tsconfig with an extends chain
  • experimentalDecorators: true inherited from the base config
  • --tsconfig-override with an extends chain (both the decorator behaviour and the absence of the "directory mismatch" warning)
USE_SYSTEM_BUN=1 bun test test/regression/issue/30477.test.ts  → 2 fail
bun bd test test/regression/issue/30477.test.ts                → 3 pass

Existing related tests all still pass (40 tests across decorators.test.ts, es-decorators.test.ts, tsconfig-override.test.ts, #27575, #27526, transpiler-tsconfig-uaf.test.ts).

PR #26436 added the experimental_decorators field on TSConfigJSON and
wired up the parse/consume sites, but forgot the merge step in
dirInfoUncached's extends-chain loop — emit_decorator_metadata is
OR-merged on the line just above it, experimental_decorators is not.
Any tsconfig that sets experimentalDecorators:true and also extends a
parent that doesn't set it silently loses the flag, so Bun emits
stage-3 decorators where legacy ones are expected (and reflect-metadata
throws).

Two drive-by fixes for --tsconfig-override while we're in here:

- transpiler.configureLinker now falls back to enclosing_tsconfig_json
  when the top-level directory has no tsconfig.json of its own. The
  override attaches to the root-directory DirInfo (parent == null),
  children only see it through the enclosing pointer, so decorator
  defaults were read from a null tsconfig.

- parseTSConfig for the override gets .invalid as its dirname_fd
  instead of the current directory's fd. The override path often lives
  somewhere that isn't a child of the dir we're iterating, which
  tripped the openat(dirname_fd, basename(path)) fallback in cache.zig
  and printed a bogus "Internal error: directory mismatch" warning.
@robobun

robobun commented May 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

@coderabbitai

coderabbitai Bot commented May 11, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Make decorator-related TSConfig fields nullable, change extends merge to per-key override semantics, avoid directory-FD mismatches for --tsconfig-override, wire merged optional flags into transpiler/runtime defaults, and add regression tests covering extends and override cases.

Changes

TypeScript Config Merge and Override Handling

Layer / File(s) Summary
Data Shape
src/resolver/tsconfig_json.zig
Make emit_decorator_metadata and experimental_decorators nullable ?bool (null = not set).
Config Merge
src/resolver/resolver.zig
During extends merging, overwrite merged decorator flags only when a parent config explicitly sets them (per-key override), rather than OR-accumulating.
Finalize Result
src/resolver/resolver.zig
finalizeResult derives decorator options from the merged tsconfig with orelse false when optional fields are unset.
Resolver Override FD Handling
src/resolver/resolver.zig
dirInfoUncached computes is_override and passes .invalid for tsconfig_dir_fd for top-level --tsconfig-override cases (unless store_file_descriptors is active and not top-level).
Transpiler: TSConfig Source Selection
src/bundler/transpiler.zig
configureLinkerWithAutoJSX prefers root_dir.tsconfig_json and falls back to root_dir.enclosing_tsconfig_json; uses chosen tsconfig to default JSX/options and decorator flags (missing → false). runEnvLoader uses dir_info.tsconfig_json or dir_info.enclosing_tsconfig_json for JSX merge.
Runtime / ParseOptions Wiring
src/runtime/api/JSTranspiler.zig
TransformTask.run and getParseResult set ParseOptions.experimental_decorators and ParseOptions.emit_decorator_metadata from optional tsconfig fields using orelse false.
Tests: regression coverage
test/regression/issue/30477.test.ts
Adds tests that assert legacy vs stage-3 decorator emission across extends chains, child overrides for experimentalDecorators and emitDecoratorMetadata, and correct behavior of --tsconfig-override without directory-mismatch stderr.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the main change: fixing experimentalDecorators merging across tsconfig extends chains in the resolver.
Description check ✅ Passed The description comprehensively covers the problem, root cause, fix, drive-by fixes, and verification. It follows the template structure with clear sections explaining the issue and solution.
Linked Issues check ✅ Passed The PR addresses all objectives from issue #30477: preserving experimentalDecorators across extends chains, eliminating spurious warnings, respecting overrides via fallback to enclosing_tsconfig_json, and restoring pre-1.3.10 decorator behavior with comprehensive test coverage.
Out of Scope Changes check ✅ Passed All changes are directly aligned with fixing #30477. The resolver.zig and transpiler.zig changes add the missing merge logic and fallback behavior; tsconfig_json.zig models optional booleans to enable proper merging; JSTranspiler.zig applies the flags correctly; the test file adds comprehensive regression coverage. No extraneous modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. fix(resolver): resolve tsconfig extends with package specifiers via node_modules #27248 - Also adds the missing experimentalDecorators merge in the tsconfig extends chain (as part of a larger fix for tsconfig extends resolution via node_modules)

🤖 Generated with Claude Code

@robobun

robobun commented May 11, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks bot, good catch. #27248 overlaps on the experimental_decorators merge fix (one line in the same loop, both pre-date each other), but the two PRs are targeting different root causes:

Merging either should close #30477. If #27248 lands first, this PR still has the two --tsconfig-override fixes; if this lands first, #27248 still needs the bare-specifier resolver.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/resolver/resolver.zig`:
- Around line 4254-4255: The OR-merge for merged_config.emit_decorator_metadata
and merged_config.experimental_decorators makes a parent true sticky; change
these fields to an optional/tri-state (e.g., ?bool or a presence flag) and
update the merge logic so the merged value is: use the child's explicit value
when present (even if false), otherwise inherit the parent's value, rather than
OR-ing; adjust any places that read these flags to treat "unset" as nil/absent
and to interpret the resolved
merged_config.<emit_decorator_metadata|experimental_decorators> after merging.

In `@test/regression/issue/30477.test.ts`:
- Around line 1-8: The current multi-line prose header should be replaced with
the repo's two-line regression header: change the comment block at the top of
the test to two lines — first line the full GitHub issue URL (e.g. //
https://github.com/oven-sh/bun/issues/30477) and second line a single short
description of the bug (e.g. // experimentalDecorators lost when tsconfig
extends another), removing the extra explanatory lines so the file follows the
standard pattern used in test/regression/issue/.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2593ae6b-f433-4fb2-95e3-b875c662118c

📥 Commits

Reviewing files that changed from the base of the PR and between 450072b and 7c8ce85.

📒 Files selected for processing (3)
  • src/bundler/transpiler.zig
  • src/resolver/resolver.zig
  • test/regression/issue/30477.test.ts

Comment thread src/resolver/resolver.zig Outdated
Comment thread test/regression/issue/30477.test.ts Outdated
Comment thread src/resolver/resolver.zig Outdated
Comment thread test/regression/issue/30477.test.ts Outdated
Comment thread src/bundler/transpiler.zig
robobun added 2 commits May 11, 2026 08:38
Address review: the OR-merge made parent `true` sticky, so a child
tsconfig could not turn off `experimentalDecorators` or
`emitDecoratorMetadata` inherited from its base. TypeScript's
`extends` semantics are per-key override — a child's explicit value
wins, even when it's `false`.

Change the two fields on TSConfigJSON from `bool` to `?bool` so the
merge loop can distinguish "child didn't say" (null) from "child
explicitly set false". Consumers (resolver.dirInfoCached,
transpiler.configureLinker, JSTranspiler) unwrap with `orelse false`.

Two new test cases pin down the override semantics:
- child `experimentalDecorators: false` overrides parent `true`
  (base enables legacy decorators, child disables them → stage-3 emit).
- child `emitDecoratorMetadata: false` overrides parent `true`
  (bundler output must not contain `__legacyMetadataTS` calls).
- Tests use test.concurrent — each runs in its own tempDir with no
  shared state, so they can overlap (2.9s vs 5.6s sequential).
- runEnvLoader in transpiler.zig uses the same enclosing_tsconfig_json
  fallback as configureLinkerWithAutoJSX, for consistency across the
  two top-level-dir lookups. Practically a no-op for the
  --tsconfig-override path (configureLinker already assigns
  options.jsx fully before this runs), but keeps the two call sites in
  sync.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/regression/issue/30477.test.ts`:
- Around line 144-150: Move the stdout assertion before the exit code assertion
so failures show actual output: place
expect(stdout).not.toContain("__legacyMetadataTS") before
expect(exitCode).toBe(0); keep the existing stderr logging (the if (exitCode !==
0) { console.log("stderr:", stderr); }) in place so stderr is still printed on
non-zero exits, but perform the stdout check prior to asserting exitCode.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fdd4bc41-6015-4d2b-af92-7cb82c5742b6

📥 Commits

Reviewing files that changed from the base of the PR and between 7c8ce85 and 2da0a85.

📒 Files selected for processing (5)
  • src/bundler/transpiler.zig
  • src/resolver/resolver.zig
  • src/resolver/tsconfig_json.zig
  • src/runtime/api/JSTranspiler.zig
  • test/regression/issue/30477.test.ts

Comment thread test/regression/issue/30477.test.ts Outdated
Comment thread test/regression/issue/30477.test.ts
- Tests 1-3 now assert `stderr === ""` so an unexpected resolver or
  transpiler warning on the non-override extends-chain paths fails the
  test instead of passing silently. Test 3 (override) keeps its
  targeted `not.toContain("directory mismatch")` — more specific than
  strict-equality for that regression.
- Bundler test asserts stdout before exitCode so a mismatch surfaces
  the actual build output in the failure diff.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/regression/issue/30477.test.ts`:
- Around line 46-48: Replace the unconditional stderr assertions in the test
(the expect(stdout).toBe("legacy\nOK\n"); expect(stderr).toBe("");
expect(exitCode).toBe(0); block and the similar blocks at the other noted
locations) with the conditional-on-failure pattern used later in the file: keep
the stdout and exitCode assertions, but only assert on stderr when the child
failed (e.g., if (exitCode !== 0) { expect(stderr).toBe(/* expected error output
*/); }). Update the three unconditional expect(stderr).toBe("") calls to follow
this conditional pattern so ASAN warnings on stderr won't cause flaky failures.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b4a39b5b-a7fd-4944-86f8-1639b3751261

📥 Commits

Reviewing files that changed from the base of the PR and between f327e99 and cebfafe.

📒 Files selected for processing (1)
  • test/regression/issue/30477.test.ts

Comment thread test/regression/issue/30477.test.ts
robobun added 2 commits May 11, 2026 09:18
…strict empty check)

Windows CI fails the strict `expect(stderr).toBe("")` — the test/
runtime on that platform emits harmless informational lines to stderr
that we don't want to gate the assertion on. Switch to the
conditional-on-failure pattern documented in test/CLAUDE.md: only
pin stderr when the exit code is non-zero, which still surfaces the
full stderr in the failure diff without making the green path
flaky.
Previously used `bun build --target bun` + a grep for `__legacyMetadataTS`
in the bundled output. That helper-function name is an implementation
detail of the runtime — if Bun ever renames or inlines it the test
breaks silently.

Switch to a pure-runtime check: the test script installs a
`Reflect.metadata` shim that records the class/key pair into a WeakMap,
then prints whether the map has an entry for `Foo.prototype`. With
`emitDecoratorMetadata: false` (child override), the shim is never
invoked and the map stays empty; with the baseline-buggy OR-merge
behavior, base's `true` wins and the shim fires. That means this case
still fails against system bun and still passes against the fix, but
doesn't depend on any emitted identifier name.
Comment thread test/regression/issue/30477.test.ts Outdated
Comment thread src/resolver/tsconfig_json.zig
@robobun

robobun commented May 11, 2026

Copy link
Copy Markdown
Collaborator Author

Diff is green; CI failures on this PR were on unrelated flaky lanes only, none touching the resolver/tsconfig/transpiler code here:

  • Build 53258 (windows x64, test-bun shard): tsconfig-uaf.test.ts subprocess timeout — pre-existing Windows flake
  • Build 53266 (windows x64 + windows x64-baseline): test/js/bun/test/parallel/test-http-should-emit-close-when-connection-is-aborted.ts timed out on parallel shard 3/8. Windows 11 aarch64 hit the same test, retried, passed. Unrelated HTTP-abort test, no tsconfig/decorator surface.
  • Build 53270 (debian-13 x64-asan, 1/287 jobs): test/js/node/test/parallel/test-worker-nested-uncaught.js — classic panic: EventLoop.enqueueTaskConcurrent: VM has terminated nested-Worker teardown race. git log --grep shows this exact flake has been retriggered on main before (0b0fe9db98).

Used my one re-roll on build 53270. Handing off — needs a maintainer to merge.

@harochau

Copy link
Copy Markdown

the test cases look solid

heikki added a commit to heikki/karttapallo that referenced this pull request May 13, 2026
Tried bumping to 1.18.1, but the bundled Bun (1.3.13) emits TC39
standard decorators from Bun.build() regardless of experimentalDecorators
in tsconfig, breaking Lit's legacy @property/@state/@query/@consume.
A TC39 migration is non-trivial (accessor keyword everywhere, plus
subtle @query/@consume initialization-order issues we couldn't tame in
one pass). Stay on 1.16.0 until oven-sh/bun#30478 lands.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@robobun

robobun commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: this PR's implementation lives entirely in Zig source files that have since been removed from the tree as part of the Rust migration. The change can no longer merge cleanly and the files it edits no longer exist on main.

If the underlying issue is still present, it will need a fresh fix against the Rust implementation.

@robobun robobun closed this Jun 26, 2026
heikki added a commit to heikki/karttapallo that referenced this pull request Aug 23, 2026
Electrobun 2.x replaces the CLI with Hutch and bundles through Cottontail,
which honours experimentalDecorators — the built view bundle emits
__legacyDecorateClassTS, so Lit's decorators work and the 1.16.0 pin from
ADR-0001 is retired. Its unblock condition never could have been met:
oven-sh/bun#30478 was closed unmerged when the Rust migration deleted the
files it patched. The main process stays Bun (bun:ffi, Bun.serve), which
electrobun pins at 1.4.0 and packages itself. Details in ADR-0017.

The SDK now lives in a generated .hutch/devkit rather than node_modules, so
tsconfig maps electrobun/* there, CI syncs before typecheck, and @types/three
— only ever needed for electrobun 1.16's untyped three import — is dropped.
The config's tsconfig-paths plugin goes with it: 2.x serializes the config, and
Cottontail's bundler reads tsconfig paths itself.

maplibre-gl 6 is ESM-only and removed the internal map.transform. The custom
points layer reads its projection data off the render input instead, and the
popup's hand-rolled globe silhouette mask is replaced by maplibre's own
locationOccludedOpacity — a popup behind the globe now fades rather than being
clipped. GeoJSONSource.setData returns a promise in 6, marked void at the
twelve fire-and-forget call sites. The bundle now carries import.meta.url, so
index.html loads it as a module and the worker ships beside it.

Also: prettier 3.9, eslint 10.9 + eslint-config-love 155, @lit-labs/signals
0.3, playwright 1.62 (run e2e:install once), and patch bumps. TypeScript 7 is
skipped — typescript-eslint refuses to load against it, and running lint on a
second, separate TS 6 compiler would leave lint and typecheck disagreeing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019u22Bj8GxveBJkHLwxHpy8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bun 1.3.10 breaks --tsconfig-override flag

2 participants